[339] Rename plantWoodCStorageDelta to plantWoodCAccountingDelta by ANAMASGARD · Pull Request #344 · PecanProject/sipnet

ANAMASGARD · 2026-05-27T14:04:45Z

Summary

FIXES #339.

Issue #339 refers to plantWoodStorageC; the actual SIPNET identifier was plantWoodCStorageDelta (output column: nppStorage). This PR renames the internal state/restart field to plantWoodCAccountingDelta to reflect that it tracks carbon changes with no associated nitrogen (NPP averaging lag / bookkeeping), not a storage pool.

No change to model equations or numeric results — rename and output formatting only.

Changes (including maintainer review feedback)

Internal/restart: plantWoodCStorageDelta → plantWoodCAccountingDelta (envi.plantWoodCAccountingDelta in checkpoints). Legacy envi.plantWoodCStorageDelta keys are rejected (no backward compatibility).
sipnet.out debug column: nppStorage → cnDelta (shorter debug-only name per review). Internal field name unchanged.
plantWoodC column: name unchanged; reports total wood C (structural + accounting term).
Docs: plantWoodC footnote [^2] in model-outputs; debug column removed from user docs; internal code names removed from model-structure prose; CHANGELOG updated.

Test plan

make testbuild
make unit (21/21 pass, including testOutputHeader and restart suite)
make smoke (5/5 sipnet; baselines updated for cnDelta column padding)
make test
python tools/smoke_check.py run verbose base (results below)

smoke_check results

Running tests: niwot, russell_1, russell_2, russell_3, russell_4
Changed working directory to: /home/linux/sipnet/tests/smoke

******************
Running test niwot
******************
Number of columns has changed for test without a header; no comparison can be performed. Skipping.
Note: git col info here may be out of date!
len git cols: 36  len new: 35

**********************
Running test russell_1
**********************
Columns have changed!
Removed columns from git: ['nppStorage']
New columns: ['cnDelta']
Comparing common columns
No differences found

**********************
Running test russell_2
**********************
Columns have changed!
Removed columns from git: ['nppStorage']
New columns: ['cnDelta']
Comparing common columns
No differences found

**********************
Running test russell_3
**********************
Columns have changed!
Removed columns from git: ['nppStorage']
New columns: ['cnDelta']
Comparing common columns
No differences found

**************************************
Skipping russell_4 (found 'skip' file)
**************************************

Full output also saved locally as smoke_check.txt at repo root.

Review responses

cnDelta: Renamed debug output column from plantWoodCAccountingDelta to cnDelta; internal field/restart key unchanged.
plantWoodC docs: Added footnote [^2] clarifying total wood C = structural + accounting.
Debug column docs: Removed from model-outputs.md as debug-only.
model-structure: Removed internal identifier from prose; kept $C_{\text{wood,accounting}}$ subscript.
Legacy restart key: Removed per @dlebauer feedback (prior commit a03a481).

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR renames SIPNET’s internal “wood storage delta” field to clarify that it is an accounting term (carbon not coupled to nitrogen), and propagates the rename through outputs, restart serialization, tests, and docs.

Changes:

Renamed plantWoodCStorageDelta → plantWoodCAccountingDelta across model code and tests.
Renamed the sipnet.out column nppStorage → plantWoodCAccountingDelta and updated smoke-check tooling and golden outputs.
Added backward-compatible restart reading for legacy checkpoints using envi.plantWoodCStorageDelta.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tools/smoke_check.py	Updates expected output column list to match the renamed output field.
tests/smoke/russell_4/sipnet.out	Updates smoke-test golden output header for the renamed column.
tests/sipnet/test_restart_infrastructure/mock_state.c	Updates restart infra test scaffolding for the renamed environment field.
tests/sipnet/test_events_types/testEventHarvest.c	Updates event test initialization to the renamed environment field.
src/sipnet/state.h	Renames the environment struct field and clarifies its meaning in comments.
src/sipnet/sipnet.c	Renames output column header and replaces all uses of the old field name.
src/sipnet/restart.c	Writes restart checkpoints with the new key and reads legacy checkpoints with the old key.
src/sipnet/events.c	Updates harvest event calculations to use the renamed field.
src/sipnet/balance.c	Updates mass-balance totals and clarifies nitrogen accounting comment.
docs/user-guide/model-outputs.md	Documents the new output column name and the prior name.
docs/parameters.md	Updates parameter documentation to the new accounting-delta terminology.
docs/model-structure.md	Updates model-structure narrative/equations to reflect accounting delta terminology.
docs/developer-guide/restart-checkpoint.md	Documents backward-compatible restart key handling post-rename.
docs/CHANGELOG.md	Adds changelog entry describing the rename and restart compatibility behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

 #define RESTART_SCHEMA_VERSION "1.0"
 #define RESTART_FLOAT_EPSILON 1e-8
+#define LEGACY_ENVI_PLANT_WOOD_C_STORAGE_DELTA "envi.plantWoodCStorageDelta"
+#define ENVI_PLANT_WOOD_C_ACCOUNTING_DELTA_INDEX 12


dlebauer · 2026-05-27T18:18:37Z

  state->enviPF[10] = (StateField){"envi.litterN",                FT_DOUBLE, &envi.litterN,                0};
  state->enviPF[11] = (StateField){"envi.plantStorageN",          FT_DOUBLE, &envi.plantStorageN, 0};
-  state->enviPF[12] = (StateField){"envi.plantWoodCStorageDelta", FT_DOUBLE, &envi.plantWoodCStorageDelta, 0};
+  state->enviPF[12] = (StateField){"envi.plantWoodCAccountingDelta", FT_DOUBLE, &envi.plantWoodCAccountingDelta, 0};


I don't think we want to support the legacy key

Agreed, it has not been around long enough (and is an internal detail)

dlebauer · 2026-05-27T18:18:58Z

+    if (setLegacyPlantWoodCAccountingDelta(
+            restartIn, key, value,
+            &state->enviPF[ENVI_PLANT_WOOD_C_ACCOUNTING_DELTA_INDEX])) {


I don't think we want to support the legacy key

Alomir · 2026-06-03T18:41:58Z


  fprintf(out, "%4d %3d %5.2f %10.2f %10.2f %12.2f ", year, day, time,
-          (envi.plantWoodC + envi.plantWoodCStorageDelta), envi.plantLeafC,
+          (envi.plantWoodC + envi.plantWoodCAccountingDelta), envi.plantLeafC,


For legacy reasons, I think we want to leave that column named plantWoodC.

As for nppStorage, since it is a debug column, I'd like something shorter. How about just cnDelta

dlebauer

@ANAMASGARD - thank you for your contribution!

I made a few comments, but will defer to @Alomir to determine what, if anything, should be changed before merging.

dlebauer · 2026-05-27T18:18:37Z

  state->enviPF[10] = (StateField){"envi.litterN",                FT_DOUBLE, &envi.litterN,                0};
  state->enviPF[11] = (StateField){"envi.plantStorageN",          FT_DOUBLE, &envi.plantStorageN, 0};
-  state->enviPF[12] = (StateField){"envi.plantWoodCStorageDelta", FT_DOUBLE, &envi.plantWoodCStorageDelta, 0};
+  state->enviPF[12] = (StateField){"envi.plantWoodCAccountingDelta", FT_DOUBLE, &envi.plantWoodCAccountingDelta, 0};


I don't think we want to support the legacy key

dlebauer · 2026-05-27T18:18:58Z

+    if (setLegacyPlantWoodCAccountingDelta(
+            restartIn, key, value,
+            &state->enviPF[ENVI_PLANT_WOOD_C_ACCOUNTING_DELTA_INDEX])) {


I don't think we want to support the legacy key

Per review on PecanProject#344: do not read envi.plantWoodCStorageDelta from checkpoints. Update docs and add test that the obsolete key is rejected.

ANAMASGARD · 2026-05-30T15:13:05Z

Thanks @dlebauer for the review — addressed in this new commit

Removed legacy envi.plantWoodCStorageDelta restart handling; checkpoints must use envi.plantWoodCAccountingDelta. The old key now fails with unknown key.
Updated CHANGELOG and restart docs per your suggested wording.
Added testObsoletePlantWoodCStorageDeltaKeyFails in the restart suite to lock in that behavior.

Left the plantWoodC output column naming as-is (it still reports total wood C = structural + accounting delta). Happy to follow up with a plantWoodCTotal rename if you prefer that in a separate PR.

Alomir

Great work! Some minor tweaks suggested, a few looking for input from @dlebauer.

Also - please run the smoke_check utility and include the results (as an attached file) in the PR description. Use this command:

python tools/smoke_check.py run verbose base > smoke_check.txt

Alomir · 2026-06-03T18:08:23Z

+|    | $C_{\text{wood,accounting}}$ | plantWoodCAccountingDelta | Wood carbon accounting delta (N-lag term; not N-coupled structural wood) | g C m$^{-2}$  |
+
+Note: this column was previously named `nppStorage`.


I appreciate trying to update the docs! However, this is a debug-only column that will likely be removed before our final deadline.

@dlebauer : do you think this column is worth documenting?

Also - the plantWoodC row needs a tweak to indicate that it is the total of "regular" plantWoodC and the accounting term. This might be best with a footnote, like soilWetnessFrac.

Alomir · 2026-06-03T18:21:50Z

-Starting in SIPNET v2.1, to support mass balance tracking, this storage is explicitly tracked as a separate pool called
-$C_{\text{wood,storage}}$, which is initialized to zero. We can represent this storage of carbon as:
+Starting in SIPNET v2.1, to support mass balance tracking, this term is explicitly tracked as
+$C_{\text{wood,accounting}}$ (`plantWoodCAccountingDelta`), which is initialized to zero. We can represent this


Suggested change

$C_{\text{wood,accounting}}$ (`plantWoodCAccountingDelta`), which is initialized to zero. We can represent this

$C_{\text{wood,accounting}}$, which is initialized to zero. We can represent this

We don't include internal naming in this doc

@dlebauer : suggestions for the subscript here? accounting is accurate in some sense, but perhaps not the best descriptor. 'non-N', or some such, maybe?

Alomir · 2026-06-03T18:41:58Z


  fprintf(out, "%4d %3d %5.2f %10.2f %10.2f %12.2f ", year, day, time,
-          (envi.plantWoodC + envi.plantWoodCStorageDelta), envi.plantLeafC,
+          (envi.plantWoodC + envi.plantWoodCAccountingDelta), envi.plantLeafC,


For legacy reasons, I think we want to leave that column named plantWoodC.

As for nppStorage, since it is a debug column, I'd like something shorter. How about just cnDelta

Alomir · 2026-06-03T18:45:17Z

      print("Comparing common columns")
  else:
-    cols = 'year day time plantWoodC plantLeafC woodCreation soil coarseRootC fineRootC litter soilWater soilWetnessFrac snow npp nee cumNEE gpp rAboveground rSoil rRoot ra rh rtot evapotranspiration fluxestranspiration minN soilOrgN litterN n2o nLeaching nFixation nUptake ch4 nppStorage bcdeltaC bcdeltaN'
+    cols = 'year day time plantWoodC plantLeafC woodCreation soil coarseRootC fineRootC litter soilWater soilWetnessFrac snow npp nee cumNEE gpp rAboveground rSoil rRoot ra rh rtot evapotranspiration fluxestranspiration minN soilOrgN litterN n2o nLeaching nFixation nUptake ch4 plantWoodCAccountingDelta bcdeltaC bcdeltaN'


Good catch!

The wood N-lag term is an accounting delta for carbon not coupled to nitrogen, not a storage pool. Renames the Envi field, sipnet.out column (was nppStorage), restart key, and docs; legacy restart keys remain readable.

Per review on PecanProject#344: do not read envi.plantWoodCStorageDelta from checkpoints. Update docs and add test that the obsolete key is rejected.

Rename sipnet.out debug column to cnDelta per maintainer feedback while keeping internal/restart key plantWoodCAccountingDelta. Add plantWoodC footnote, remove debug column from user docs, and fix ch4/cnDelta spacing.

ANAMASGARD · 2026-06-10T12:36:27Z

@Alomir and @dlebauer I have addressed remaining review feedbacks :

Renamed debug sipnet.out column to cnDelta (internal/restart key remains plantWoodCAccountingDelta)
Added plantWoodC footnote [^2] in model-outputs; removed debug column from user docs
Removed internal code name from model-structure prose
Updated smoke_check.py column list and smoke baselines
Fixed ch4/cnDelta spacing when cnDelta is negative
Ran `make test` (21 unit + 5 smoke) — all pass
smoke_check results added to PR description

Ready for re-review @Alomir

Copilot AI review requested due to automatic review settings May 27, 2026 14:04

ANAMASGARD requested review from Alomir, dlebauer, mdietze and mswilburn as code owners May 27, 2026 14:04

Copilot AI reviewed May 27, 2026

View reviewed changes

dlebauer reviewed May 27, 2026

View reviewed changes

Alomir requested changes Jun 3, 2026

View reviewed changes

ANAMASGARD added 2 commits June 10, 2026 16:58

[339] Rename plantWoodCStorageDelta to plantWoodCAccountingDelta

a10c4b7

The wood N-lag term is an accounting delta for carbon not coupled to nitrogen, not a storage pool. Renames the Envi field, sipnet.out column (was nppStorage), restart key, and docs; legacy restart keys remain readable.

[339] Drop legacy restart key for plantWoodCAccountingDelta

a03a481

Per review on PecanProject#344: do not read envi.plantWoodCStorageDelta from checkpoints. Update docs and add test that the obsolete key is rejected.

ANAMASGARD force-pushed the rename/plantWoodCAccountingDelta-closes-339 branch from 9b3e7db to a03a481 Compare June 10, 2026 11:29

[339] Address review: rename debug column to cnDelta and update docs

8af1cf5

Rename sipnet.out debug column to cnDelta per maintainer feedback while keeping internal/restart key plantWoodCAccountingDelta. Add plantWoodC footnote, remove debug column from user docs, and fix ch4/cnDelta spacing.

ANAMASGARD requested review from Alomir and dlebauer June 10, 2026 12:49

Merge branch 'master' into rename/plantWoodCAccountingDelta-closes-339

97b395e

		\| \| $C_{\text{wood,accounting}}$ \| plantWoodCAccountingDelta \| Wood carbon accounting delta (N-lag term; not N-coupled structural wood) \| g C m$^{-2}$ \|

		Note: this column was previously named `nppStorage`.

	$C_{\text{wood,accounting}}$ (`plantWoodCAccountingDelta`), which is initialized to zero. We can represent this
	$C_{\text{wood,accounting}}$, which is initialized to zero. We can represent this

Conversation

ANAMASGARD commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes (including maintainer review feedback)

Test plan

smoke_check results

Review responses

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlebauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ANAMASGARD commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alomir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ANAMASGARD commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ANAMASGARD commented May 27, 2026 •

edited

Loading

ANAMASGARD commented May 30, 2026 •

edited

Loading

ANAMASGARD commented Jun 10, 2026 •

edited

Loading